- 
                Notifications
    
You must be signed in to change notification settings  - Fork 60
 
fix: use wrapped context's err when <-ctx.Done() case is executed #463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5f9ce77    to
    5362b47      
    Compare
  
    5362b47    to
    7aa279e      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patch! I have a few comments.
| 
               | 
          ||
| ### Fixed | ||
| 
               | 
          ||
| - Use wrapped context err when <-ctx.Done() case is executed | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a link to the issue in the changelog entry.
| - Use wrapped context err when <-ctx.Done() case is executed | |
| - Use wrapped context err when <-ctx.Done() case is executed (#457). | 
| 
               | 
          ||
| ### Changed | ||
| 
               | 
          ||
| ### Fixed | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, move it into Changed or Added section of the CHANGELOG.md. It does not fixes anything, but looks like an improvment.
| // Instruct msgpack to pack this struct as array, so no custom packer | ||
| // is needed. | ||
| _msgpack struct{} `msgpack:",asArray"` //nolint: structcheck,unused | ||
| _msgpack struct{} `msgpack:",asArray"` // nolint: structcheck,unused | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| // Concurrency: 32, | ||
| // RateLimit: 4*1024, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| } | ||
| 
               | 
          ||
| /////////////////// | ||
| // ///////////////// | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| conn := test_helpers.ConnectWithValidation(t, dialer, opts) | ||
| defer conn.Close() | ||
| req := NewPingRequest().Context(nil) //nolint | ||
| req := NewPingRequest().Context(nil) // nolint | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| Version: ProtocolVersion(1), | ||
| Features: []iproto.Feature{iproto.IPROTO_FEATURE_STREAMS}, | ||
| }).Context(nil) //nolint | ||
| }).Context(nil) // nolint | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| Version: ProtocolVersion(1), | ||
| Features: []iproto.Feature{iproto.IPROTO_FEATURE_STREAMS}, | ||
| }).Context(ctx) //nolint | ||
| }).Context(ctx) // nolint | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert the unnecessary change.
| fut.SetError(fmt.Errorf( | ||
| "context is done (request ID %d): %w", | ||
| fut.requestId, context.Cause(ctx), | ||
| )) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a test that we could check the context error existence with errors.As/errors.Is.
| 
           Closed with #476 .  | 
    
Related issues:
#457